Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add callback functionality for input events #293

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

trondhumbor
Copy link

Allows adding custom callback functions, which will be called when glfw fires an input event.

@dusk125
Copy link
Collaborator

dusk125 commented Sep 2, 2021

Thanks for the PR! Can you explain a bit of your use-case/thought process behind this change please? I'm interested to see how this differs from using the existing pixel.Window input functions.

@dusk125 dusk125 added the feature label Sep 2, 2021
@cebarks
Copy link
Collaborator

cebarks commented Sep 2, 2021

I was thinking this feature would be nice but came to the conclusion most things i would do with callbacks i could just do with checking an input loop. However, the best idea I had was for UI elements (checking every mouse click if you're over a button on the screen, etc.)

@trondhumbor
Copy link
Author

The intention was to provide a way to make use of the callback methods already existing in glfw. Instead of exposing them raw, I wrapped them so that glfw elements won't leak into the pixel library. One of the main benefits with this change, as I see it, is that you can avoid using an input loop for something that is event-driven "under-the-hood".

})

w.window.SetCharCallback(func(_ *glfw.Window, r rune) {
w.tempInp.typed += string(r)
for _, cbfun := range w.callbacks.charCallbacks {
go cbfun(r)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason you are doing this asynchronously with go instead of calling them sequentially? I feel like this could be an issue if callbacks overlap with each other. Not sure if that's a valid fear or not however. Would like to hearinput from you and @dusk125 on this...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Say you register multiple callbacks for an event. Calling the callbacks asynchronously was done as to not keep one callback waiting for the previous one to finish. However, I see the reason for calling them sequentially, so that might be something that we could change.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps maybe a waitgroup then?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preference would be to call them sequentially, and leave it to the handler to do what it needs to async. Especially handling typing, things could get out of order which would be annoying.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe have async as a bool option somehow?

@dusk125
Copy link
Collaborator

dusk125 commented Sep 4, 2021

So the thing I'm struggling with here is that I don't know if this actually buys you anything. GLFW might be able to send events async, but OpenGL is still single threaded and basically all calls in Pixel that go touch OpenGL are forcibly done in the main thread.

The thing I'm not sure about (given my inexperience with GLFW) is if the callbacks are actually done async or if they're also called single threaded-ly; maybe you can speak to that? In Pixel, we call glfw.PollEvents (on the main thread) and then update our internal structure.

@cebarks
Copy link
Collaborator

cebarks commented Sep 5, 2021

@trondhumbor what's your use case for this feature? i'm curious

@Chillance
Copy link

Chillance commented Sep 19, 2021

Related to https://github.com/dusk125/pixelui/issues ? If so, would be nice to get this merged so the others can also be.

@dusk125
Copy link
Collaborator

dusk125 commented Sep 19, 2021

Related to https://github.com/dusk125/pixelui/issues ? If so, would be nice to get this merged so the others can also be.

Thanks for reminding me to go through the PixelUI issues :D All of those issues were waiting on PRs that have now been accepted into Pixel (they've now been closed); that is to say, this isn't related to the issues I was having in PixelUI.

@Chillance
Copy link

Related to https://github.com/dusk125/pixelui/issues ? If so, would be nice to get this merged so the others can also be.

Thanks for reminding me to go through the PixelUI issues :D All of those issues were waiting on PRs that have now been accepted into Pixel (they've now been closed); that is to say, this isn't related to the issues I was having in PixelUI.

PixelUI is fantastic. Great addition to fantastic Pixel. Started to use PixelUI myself and noticed it needs to upgrade the underlying imgui-go (made an issue for it) Anyway, this is about Pixel so I hope this PR will be merged soon too.

@duysqubix
Copy link
Contributor

@dusk125

Is this PR still relevant?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants